-
Notifications
You must be signed in to change notification settings - Fork 9
Adapt to JuliaSyntax MacroName change
#71
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Ah yeah some of the changes you're forced to make here make me wonder whether the JuliaSyntax changes were good tradeoffs. But I'm not sure I know exactly what you meant with this comment - could you expand? |
|
I think that some piece of code should be responsible for producing the most useful possible AST for macro writers. (This also probably means a good AST for lowering, JETLS, and other consumers that don't use the green tree much.) I also think we're at risk of failing to assign this responsibility to anything. What we currently do is pretty close to ideal! To me, "useful" should also include:
I agree that lowering-like transforms shouldn't be the job of a parser, but I can't think of any better place to produce the most-useful-AST than JuliaSyntax. We could assign that responsibility to a new pre-macro-expansion pass in lowering, but then packages wouldn't benefit, and tooling would see a different AST than macros. The best solution I have is to consider the green tree -> Side question: What was the original reason for having the |
|
As for this PR, it does make our implementation more complex, and I'm OK with closing it and using the diff as reason to change JuliaSyntax instead. I'm also OK with merging in the meantime and undoing it later, since we do need to bump our JuliaSyntax version at some point. |
Excellent points! I think there's two possible answers:
In either case, I strongly believe the underlying green tree or equivalent information should still be accessible to macros and lowering. For example, the difference between
The reason is that The old parser did this by rewriting the |
Can we have both? :) I think pattern matching is good regardless, and careful AST design makes it even better.
Oh, that makes sense. I realize our
That's a good example and I think I agree. Is your plan to have green tree access go through the SyntaxTree provenance system? That might solve the need for An alternative (though opt-in) solution to |
Yes of course :-D A all the fiddly differences in conversion to
After thinking a little more about it, no. We can't rely on I've got a lot to write about this, I'll try to capture it into an issue.
If we magically had syntax evolution this is one of the first things I'd deprecate! It's awkward to deal with and adds nothing of value over treating the |
Thanks for writing things out!
I don't quite follow. If a macro needs to tell whether its input was Putting this information in the tree or syntax flags seems it would erase the third option and make it equivalent to one of the others. Maybe that's desirable (pre-handles an edge case)? I would be 100% OK with telling authors of macros that peek at the green tree to handle the case where there is no green tree, since this way they can choose the behaviour they want. |
Ah, I think I misunderstood your question.
Basically, I see provenance as a separate concern from the data which macros are "allowed" to inspect when generating output AST. (Put another way, if a rogue macro wants to dig into the |
No worries, I should have been more clear with "green tree access through the provenance system." What I meant is representing the green tree as a SyntaxTree and adding it to our existing chain of these things, so we have
That's fair. My motivation in suggesting that macros be allowed to use the green tree is so we don't have to hand-select the set of "not semantically important but maybe important to a few macros" syntax. If we manage to standardize the green tree, it would be worth looking into making a good API, but for now I agree it would be brittle. I'm OK with hand-selecting these things and putting them in the syntax flags. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's do it
test/macros.jl
Outdated
| sprint(showerror, exc) | ||
| end == """ | ||
| MacroExpansionError while expanding @oldstyle_error in module Main.macro_test: | ||
| MacroExpansionError while expanding (macro_name oldstyle_error) in module Main.macro_test: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's convert to Expr and use the pretty printing from there rather than showing the sexpr form to users? (to be fair this was always a problem, it's just more obvious with the latest changes)
| mac_name = string(e.args[1]) | ||
| mac_name = mac_name == "@__dot__" ? "." : mac_name[2:end] | ||
| child_exprs[1] = Symbol(mac_name) | ||
| end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oof, this back and forth conversion is ugly ... at face value it makes me worry the upstream changes weren't a good tradeoff.
I guess we could delete all this if we gave up on trying to reproduce all the macro name kinds. They are mainly present to represent the source text as it is parsed ... it might be fine if they're normalized to K"Identifier" when converting Expr back to SyntaxTree?
On the other hand, it seems bad to have more cases when SyntaxTree->Expr->SyntaxTree gives a different expression given that macros can observe the difference. Hmm.
|
Ok, I've cleaned up the macro name thing. It turns out that pretty printing macro names is surprisingly messy but it's possible to co-opt the |
Update to the latest dev version of JuliaSyntax, where the only notable change was JuliaLang/JuliaSyntax.jl#572 minus changes from JuliaLang/JuliaSyntax.jl#583. Not sure how I feel about the change in general (in favour of making identifiers
K"Identifiers, against nodes that don't change semantics, in favour of cleaning up the parser), but I certainly want the latest JuliaSyntax and JuliaLowering to work together. This change continues to put identifier normalization in lowering.Some fixes were needed on on the JuliaSyntax end, see JuliaLang/JuliaSyntax.jl#595.